Skip to content

Refactor HttpRuntime usage to minimize statics usage #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 17, 2023
Merged

Refactor HttpRuntime usage to minimize statics usage #295

merged 2 commits into from
Feb 17, 2023

Conversation

@twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Feb 17, 2023

In ASP.NET Framework, HttpRuntime was used as a static holder for the runtime information. We expose this type in the adapters, but it should not be accessed directly internally due to its static nature. This currently causes sporadic test failures. Rather, internally should access the IHttpRuntime if this information is needed.

This change includes:

  • Changes UrlPath to be a class that takes in the IHttpRuntime
  • Moves the logic of VirtualPathUtility into a VirtualPathUtilityImpl that is an instance class
  • VirtualPathUtility uses a cached instance of VirtualPathUtilityImpl when needed
  • Current usage of VirtualPathUtility in the adapters is replaced with the VirtualPathUtilityImpl
  • Tests are updated to no longer require setting HttpRuntime.Current

@twsouthwick
Refactor HttpRuntime usage to minimize statics usage
fc1413c 
In ASP.NET Framework, HttpRuntime was used as a static holder for the runtime information. We expose this type in the adapters, but it should not be accessed directly internally due to its static nature. This currently causes sporadic test failures. Rather, internally should access the IHttpRuntime if this information is needed.

This change includes:

- Changes UrlPath to be a class that takes in the IHttpRuntime
- Moves the logic of VirtualPathUtility into a VirtualPathUtilityImpl that is an instance class
- VirtualPathUtility uses a cached instance of VirtualPathUtilityImpl when needed
- Current usage of VirtualPathUtility in the adapters is replaced with the VirtualPathUtilityImpl
- Tests are updated to no longer require setting HttpRuntime.Current
@twsouthwick
Copy link
Member Author

twsouthwick commented Feb 17, 2023

/cc @CZEMacLeod

@twsouthwick twsouthwick requested a review from Tratcher February 17, 2023 22:37
@twsouthwick twsouthwick merged commit e8eff1d into main Feb 17, 2023
@twsouthwick twsouthwick deleted the tasou/runtime-via-di branch February 17, 2023 23:05
@CZEMacLeod
Copy link
Contributor

CZEMacLeod commented Feb 18, 2023

Just a couple of quick notes - this should probably mention that it includes a heap of changes to add copyright information to some files not associated with HttpRuntime or the VPU stuff.
Also changes Cache to match code style (no single line ifs, and removing backing fields).
I think the ref file should not have System.Runtime.CompilerServices.CompilerGeneratedAttribute in it, as this does not match the native dll, and is an implementation detail.
That attribute should maybe be added to the excluded attributes file instead. (src/Microsoft.AspNetCore.SystemWebAdapters/Generated/ExcludedAttributes.txt)

@twsouthwick
Copy link
Member Author

twsouthwick commented Feb 21, 2023

Created #298 to remove the attribute. Good call.

All the code is inspired or taken from referencesource so not sure we need specific call outs for that. I'll follow up on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@twsouthwick @CZEMacLeod @Tratcher